Small code fixes to Placement Plugins#4213
Conversation
| } | ||
|
|
||
| // If there are not multiple spreadDomains, then there is nothing to spread across | ||
| // only spread across if there are multiple spreadDomains |
There was a problem hiding this comment.
I hope this is a clearer comment...
HoustonPutman
left a comment
HoustonPutman
left a comment
There was a problem hiding this comment.
Generally approve, thanks for the cleanup!
|
|
||
| List<WeightedNode> nodesForRequest = | ||
| weightedNodes.stream().filter(request::isTargetingNode).collect(Collectors.toList()); | ||
| weightedNodes.stream().filter(request::isTargetingNode).toList(); |
There was a problem hiding this comment.
I'm not sure if the read-only contract is different between these two options, but good to check. Same down below.
There was a problem hiding this comment.
humm, I'm hoping no differences because tests don't fail and IntelliJ recommended it! Let's see what copilot says!
There was a problem hiding this comment.
Claude looked at it and explained about the mutability thing... It's interesting that nothing in the class speaks to mutablity which feels like a trap. I mean, do I really need to think about mutability? It's like asking me to think about memory management!!! What are we? C code?
There was a problem hiding this comment.
Pull request overview
This PR makes small cleanups and typo fixes in Solr placement plugin code, primarily improving readability and modernizing a couple of Java stream usages.
Changes:
- Removes unused code in placement plugin tests and plugin internals.
- Fixes minor Javadoc/comment typos in placement plugin classes.
- Replaces
collect(Collectors.toList())withStream.toList()and refactorsinstanceofchains to aswitchpattern.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| solr/core/src/test/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactoryTest.java | Removes an unused variable in a test. |
| solr/core/src/java/org/apache/solr/cluster/placement/plugins/RandomPlacementFactory.java | Fixes a small Javadoc punctuation typo. |
| solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java | Uses toList(), refactors request-type handling to a switch, and adjusts comments. |
| solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java | Small comment rewording and simplifies a helper method signature. |
| solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementConfig.java | Javadoc tweaks; includes a newly introduced punctuation typo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Had to back out the |
Reading through this code to learn about placement plugins I fixed some typos and small code clean ups.